Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ReturnTypeWillChange to ReflectionEmbeddedProperty #8742

Merged
merged 1 commit into from
Jun 5, 2021

Conversation

derrabus
Copy link
Member

@derrabus derrabus commented Jun 5, 2021

PHP 8.1 will introduce a process to add return types to internal methods, see https://wiki.php.net/rfc/internal_method_return_types

This library extends ReflectionProperty and overrides public methods that are affected by this change. PHP 8.1 triggers deprecation warnings unless we declare that a future version of this library will add these return types as well. We can do so by adding the ReturnTypeWillChange attribute to those methods.

This PR suggests to do just that because adding the actual return types would break code that extends our classes. A future 3.0 release could add the actual return types.

@derrabus
Copy link
Member Author

derrabus commented Jun 5, 2021

The PHP CodeSniffer errors seem to be false positives.

edit: Reported as slevomat/coding-standard#1233

@greg0ire
Copy link
Member

greg0ire commented Jun 5, 2021

@derrabus let's add an ignore rule with the link above then? Or is the plan to wait for an upstream fix?

@derrabus derrabus force-pushed the bugfix/return-type-will-change branch from 0f6f28b to 6432a3e Compare June 5, 2021 20:52
@derrabus
Copy link
Member Author

derrabus commented Jun 5, 2021

let's add an ignore rule with the link above then?

Done.

@greg0ire greg0ire merged commit b880809 into doctrine:2.9.x Jun 5, 2021
@greg0ire greg0ire added this to the 2.9.3 milestone Jun 5, 2021
@greg0ire
Copy link
Member

greg0ire commented Jun 5, 2021

Thanks @derrabus !

@derrabus derrabus deleted the bugfix/return-type-will-change branch June 5, 2021 22:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants